test(studio): pin three unpinned substrate/launch safety invariants (PX0 exit-3) - #507
Merged
KnockOutEZ merged 3 commits intoAug 28, 2026
Merged
Conversation
…ded process kill
`spawn()` reports EACCES, EPERM and a Gatekeeper refusal by emitting 'error' on a
later tick, not by throwing, so runStudio's try/catch never sees it and an
unlistened event is rethrown as an uncaught exception on a CLI process that has
already detached the child and returned to the shell. The listener has always
been there; nothing asserted it, because fakeSpawn returns { on: () => undefined }.
Reuses auto-launch's real-EventEmitter fake so the ordering claim is about the one
moment the command stops being able to attach anything, not about what a later
look happens to find.
…its own root The existing arm plants '../escape', so a predicate narrowed to !/[\\/]/ stays green — and that narrowing reads as a simplification. It is not: destDir is join(root, version) and the next statement is a recursive rmSync, so '.' deletes every installed version plus the record and '..' deletes the data dir, cache DB, keys and profiles. Neither carries a separator. The fixture half-uninstalls first because a valid record short-circuits to already_present before destDir is ever computed; record-present-executable-gone is the reachable state, and is the one this file already names elsewhere. Asserts what survived rather than the outcome word: under the narrowed predicate '..' still ends in 'failed' — after wiping the data dir, then failing to write the record into the substrate/ directory it just deleted.
…k cycle Termination rests on one fact: readdirSync(withFileTypes) stats without following, so a link to a directory reports isSymbolicLink() and is judged rather than descended. The framework fixture has contained directory links but none pointing at an ancestor, so the walk finished for reasons unrelated to the rule. 'self -> .' is a legal, contained tree that must install. Descending into contained directory links — which reads as making the walk more thorough — turns it into self/self/self/... The caller is acquireSubstrate on the warmup path, unattended and with no timeout of its own. Carries a per-test timeout so a lost rule reports as a failing test rather than a runner that stopped making progress.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the PX0 exit-3 finding: three substrate/launch safety invariants that are correct in the code today but had nothing holding them there. Additive tests only —
git diff --stat src/is empty, and every arm passes againstHEADas-is.Each arm below was proved to red under the stated mutation, and the mutation reverted by reverse-edit.
1.
runStudio— the spawn error listener (tests/unit/cli/studio.test.ts)spawn()reports EACCES, EPERM and a Gatekeeper refusal by emitting'error'on a later tick, not by throwing, sorunStudio's try/catch is not in that path. An'error'with no listener is rethrown byEventEmitteras an uncaught exception — on a process that has alreadyunref'd the child and returned to the human's shell.fakeSpawnreturns{ on: () => undefined }, so nothing observed the listener. Reusesauto-launch's real-EventEmitterfakeChild, which recordslistenerCount('error')atunref()time.Mutation: delete
child.on('error', …)fromsrc/cli/studio.ts.The other 82 arms — including all six existing
runStudiocases — stayed green, which is the gap being closed. Restored:84 passed (84).2.
isSingleDirectoryName—.and..(tests/unit/studio/substrate-acquire.test.ts)The existing arm plants
../escape, so a predicate narrowed to!/[\\/]/stays green — and that narrowing reads as a simplification. It is not.destDirisjoin(root, version)and the next statement isrmSync(destDir, { recursive: true, force: true }):.→destDiris the substrate root → deletes every installed version andrecord.json..→destDiris the data dir → deletes the cache DB, keys and profilesNeither spelling carries a separator.
Two things about the fixture shape, both deliberate and both departures from the issue's sketch:
acquireSubstratereturnsalready_presentbefore it ever computesdestDir, so the literal "install a good substrate, then attempt the bad version" never reaches the delete. The reachable state is record-present / executable-gone — an interrupted uninstall or partial upgrade — which this file already names elsewhere. The record is then read back at the end, once its executable is restored, which is only possible because neither it nor its directory was removed...still ends infailed: it wipes the data dir, then fails writing the record into thesubstrate/directory it just deleted. An outcome-only arm would go green on the shape that wiped the machine.Mutation:
return Boolean(name) && !/[\\/]/.test(name);The pre-existing
../escapearm was among the 36 that stayed green. Restored:38 passed (38).3.
findEscapingLink— termination on a contained cycle (tests/unit/studio/substrate-acquire.test.ts)Termination rests on one fact:
readdirSync(withFileTypes)stats without following, so a link to a directory reportsisSymbolicLink()and is judged rather than descended. The framework fixture has contained directory links, but none points at an ancestor, so the walk finished for reasons unrelated to the rule.self -> .is a legal, contained tree that must install. Descending into contained directory links — which reads as making the walk more thorough — turns it intoself/self/self/…, insideacquireSubstrateon the warmup path, unattended and with no timeout of its own.Mutation: after the containment check passes,
if (statSync(child).isDirectory()) pending.push(child);The runaway descent hits the platform's
SYMLOOP_MAXand surfaces as afailedoutcome rather than a hang, so the pin reports as a clean red. The per-test timeout is kept anyway, so a platform where it does hang still reports as a failing test rather than as a runner that stopped making progress.Verification
Territory:
tests/unit/studio/**(lane) andtests/unit/cli/studio.test.ts(declared exception). No source, nointernal-docs/, noCLAUDE.md, noMakefile. All probe fixtures under$TMPDIR.